[TrimmableTypeMap] Use Crc64 package naming by default with LowercaseCrc64 compatibility#11193
[TrimmableTypeMap] Use Crc64 package naming by default with LowercaseCrc64 compatibility#11193
Conversation
|
@copilot I would prefer changing the prefix from |
There was a problem hiding this comment.
Note that there is the property:
<AndroidPackageNamingPolicy>LowercaseCrc64</AndroidPackageNamingPolicy>We will probably want to introduce a new one for XxHash64.
It is a slight breaking change, because people could have a hardcoded package name in a script for launching their main activity, etc.
So, I think what we did in some really old Xamarin.Android release, we changed the default but allowed projects to still use the old one.
So funny thing is that the CSC64 we have in our C++(or C#?) code and CSC64 that is in System.IO.Hashing produce different results. I decided to use the System.IO.Hashing one in the trimmable typemap so that would be a breaking change anyway. I will need to revisit this and try to make the |
|
@copilot let's respect |
Implemented in ec92067: trimmable typemap now respects
... Addressed in ec92067: added trimmable support for |
| var data = System.Text.Encoding.UTF8.GetBytes (value); | ||
| var hash = System.IO.Hashing.XxHash64.Hash (data); | ||
| return BitConverter.ToString (hash).Replace ("-", "").ToLowerInvariant (); |
There was a problem hiding this comment.
@copilot we should take inspiration from dotnet/android-tools#335 and avoid so many allocations here - use stackalloc where possible and non-allocating method overloads. Make sure to reuse the ToHexString method which is a lot more efficient than what we have right now. Also I don't think we need to hash UTF-8, we can simply do ReadOnlySpan<byte> utf16Bytes = MemoryMarshal.AsBytes(text.AsSpan()); , right? We hash a lot of strings during the build process, so let's make sure we don't unnecessarily slow down builds.
There was a problem hiding this comment.
Implemented in 8d5709b: reduced allocations in JavaPeerScanner by switching to span-based hex formatting (with stackalloc), using non-allocating XxHash64.Hash(ReadOnlySpan<byte>, Span<byte>), and removing intermediate "{ns}:{assembly}" string allocations via pooled UTF-8 buffers. I kept UTF-8 input for hash compatibility with existing outputs/policies.
simonrozsival
left a comment
There was a problem hiding this comment.
@copilot address these comments
I want to move the hashing code from JavaPeerScanner to a separate static ScannerHashingHelper class which can be better tested. I want a test that will just simply validate some known inputs to some known outputs of the legacy crc64 algorithm to prove we're maintaining backwards compatibility.
| <AndroidVersionCodePattern Condition=" '$(AndroidUseLegacyVersionCode)' != 'True' And '$(AndroidVersionCodePattern)' == '' ">{abi}{versionCode:D5}</AndroidVersionCodePattern> | ||
| <AndroidResourceGeneratorTargetName>UpdateGeneratedFiles</AndroidResourceGeneratorTargetName> | ||
| <AndroidUseApkSigner Condition=" '$(AndroidUseApkSigner)' == '' ">True</AndroidUseApkSigner> | ||
| <AndroidPackageNamingPolicy Condition=" '$(AndroidPackageNamingPolicy)' == '' ">LowercaseCrc64</AndroidPackageNamingPolicy> |
There was a problem hiding this comment.
the XxHash64 should be the default just for the trimmable typemap
There was a problem hiding this comment.
Addressed in 05cebc7: reverted the global AndroidPackageNamingPolicy default and scoped XxHash64 as the default specifically for trimmable typemap generation.
Implemented in 05cebc7: moved scanner hashing into
Handled in 05cebc7: trimmable typemap continues to respect explicit
|
|
@copilot update the PR by pulling latest main and resolve conflicts |
|
@copilot in dotnet/android-tools#338 we actually showed that
|
Done in e9ae72c. Trimmable default is now |
| Span<byte> hash = stackalloc byte [8]; | ||
| System.IO.Hashing.Crc64.Hash (utf8Buffer.Slice (0, bytesWritten), hash); | ||
| return ToHexString (hash, lowercase: true); |
There was a problem hiding this comment.
One problem is if you have "\0" and "\0\0\0\0" both with all zeros, then the crc value will be the same. Is that a problem here?
We XOR the length on this one:
There was a problem hiding this comment.
Good catch. Addressed in 7aa55cd: ToCrc64() now XORs the byte length into the CRC64 value (matching the legacy collision-avoidance approach), and I added a regression test for different-length zero-byte inputs. No UI changes (screenshot N/A).
| int slash = javaName.IndexOf ('/'); | ||
| if (slash > 0) { | ||
| return "crc64.../" + javaName.Substring (slash + 1); | ||
| return "hash.../" + javaName.Substring (slash + 1); |
There was a problem hiding this comment.
We prefixed crc64 so the algorithm was clear. Should we pick something less generic than hash?
There was a problem hiding this comment.
Addressed in 7aa55cd: normalization keeps the explicit crc64.../ prefix, so we avoid a generic hash marker and keep the algorithm intent clear. No UI changes (screenshot N/A).
ab689ec to
d71cbd8
Compare
…efault Agent-Logs-Url: https://github.com/dotnet/android/sessions/a90e2abd-ca59-4b98-a498-64caa983b1d0 Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/android/sessions/9fd6ed14-5c7b-463e-b1b0-ebefd496f067 Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/android/sessions/144010de-77de-456e-aee5-4db9e9a1a5cb Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
…seCrc64 legacy Agent-Logs-Url: https://github.com/dotnet/android/sessions/fdb58e79-42f0-4d7d-8180-2b58c52ae3f8 Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
…fers Agent-Logs-Url: https://github.com/dotnet/android/sessions/3701f7a2-7d92-4e63-8d34-f4723682c1ba Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/android/sessions/3701f7a2-7d92-4e63-8d34-f4723682c1ba Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
d71cbd8 to
0354f0d
Compare
|
@copilot resolve the merge conflicts in this pull request, also address review comments from Jon |
…ixes Agent-Logs-Url: https://github.com/dotnet/android/sessions/6064358e-ab27-46b0-a022-bc0dfde2443a Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
This updates trimmable type map generation to default to
Crc64-based package naming for unregistered types, while preserving backwards compatibility for projects that explicitly set:Package naming policy support in trimmable typemap
AndroidPackageNamingPolicyinto trimmable typemap generation.Crc64(trimmable default) →crc64...usingSystem.IO.Hashing.Crc64with length mixingLowercaseCrc64→crc64...using the existing legacy/custom CRC64 implementation for compatibility with previous namingXxHash64handling, as it no longer provides value for this path.Mono.Androidnamespace-preserving behavior unchanged.Default policy update
Crc64is now the default only for trimmable typemap generation.AndroidPackageNamingPolicydefault remainsLowercaseCrc64for non-trimmable typemap paths.Hashing implementation refactor
JavaPeerScannerinto a dedicated staticScannerHashingHelperfor easier focused testing.ScannerHashingHelper.ToCrc64()forSystem.IO.Hashing.Crc64.Crc64path to usestackallocwithnew byte[N]fallback for larger buffers (>256 bytes).crc64 ^ byteLength) inToCrc64()to avoid equal outputs for different-length all-zero inputs.Targeted test updates
crc64trimmable defaults.Crc64output,LowercaseCrc64compatibility output.XxHash64-specific scanner expectations.Crc64known-output validation into a[Theory]with multiple input/output vectors.Crc64outputs.Related plumbing updates
TrimmableTypeMapGeneratorand scanner hashing logic.mainconflict changes so trimmable typemap generation keeps both package naming policy support andMaxArrayRankplumbing.